Skip to content

Conversation

estebank
Copy link
Contributor

#95098 introduces new From impls for Vec, which can break existing
code that relies on inference when calling .as_ref(). This change
explicitly carves out a bias in the inference machinery to keep
existing code compiling, while still maintaining the new From impls.

Reported in #96074.

…e without reverting rust-lang#95098

 rust-lang#95098 introduces new `From` impls for `Vec`, which can break existing
 code that relies on inference when calling `.as_ref()`. This change
 explicitly carves out a bias in the inference machinery to keep
 existing code compiling, while still maintaining the new `From` impls.

 Reported in rust-lang#96074.
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 26, 2022
@rust-highfive
Copy link
Contributor

r? @davidtwco

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 26, 2022
@jackh726
Copy link
Member

I...don't like this. It's clever, but I think a revert #95098 is more prudent.

@estebank
Copy link
Contributor Author

@jackh726 this was a proof of concept to see what would be necessary to bias the inference machinery to get these to work without disrupting the ecosystem. I was particularly intrigued when it comes to rust-lang/rfcs#3240, as that will require checks similar to this one (not hardcoded, but the same thing).

Comment on lines +1773 to +1774
&& tcx.is_diagnostic_item(sym::AsRef, other.def_id)
&& tcx.is_diagnostic_item(sym::AsRef, victim.def_id)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These would have to be lang_item checks instead (to not lie about AsRef and Vec not being language items after this).

Comment on lines +1775 to +1776
&& other.substs.len() > 1
&& victim.substs.len() > 1
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are likely redundant with the two lines above.

&& tcx.is_diagnostic_item(sym::Vec, def.did())
{
// If this is an `AsRef` implementation that can go either way for
// `AsRef<[T]>` or `AsRef<Vec[T]>`, prefer the former.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// `AsRef<[T]>` or `AsRef<Vec[T]>`, prefer the former.
// `AsRef<[T]>` or `AsRef<Vec<T>>`, prefer the former.

@davidtwco
Copy link
Member

r? @estebank

@rust-highfive rust-highfive assigned estebank and unassigned davidtwco Apr 27, 2022
@estebank estebank closed this Apr 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants